Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[P4Testgen] BMv2 test generation improvements #4046

Merged
merged 9 commits into from
Jun 27, 2023
Merged

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jun 22, 2023

  • Use source information as the comparison operator again, not clone ID.
  • Clean up the Z3 solver. Do not keep context around and reset memory usage periodically to avoid running out of memory.
  • Reduce the PTF wait time to accelerate PTF testing.
  • Clean up the entry_restriction parser and print a warning if a restriction is not satisfiable. Also ensure argument and parameter names are consistent.
  • Change the optional implementation once again. Convert it into a ternary match that either matches exactly or is wildcarded.

@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Jun 22, 2023
@fruffy fruffy force-pushed the fruffy/benchmarking_fixes branch from f590c26 to 0d1ad79 Compare June 22, 2023 17:15
@fruffy fruffy force-pushed the fruffy/benchmarking_fixes branch from 93b379c to 3d158dc Compare June 22, 2023 23:09
@fruffy fruffy marked this pull request as ready for review June 26, 2023 20:50
Copy link
Contributor

@jnfoster jnfoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -404,10 +404,10 @@ class Test{{test_id}}(AbstractTest):
## else
ptfutils.verify_packet(self, exp_pkt, eg_port)
bt.testutils.log.info("Verifying no other packets ...")
ptfutils.verify_no_other_packets(self, self.device_id, timeout=2)
ptfutils.verify_no_other_packets(self, self.device_id, timeout=0.1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend maybe using a global variable name or a return value from a global function defined with def at the top level to get this timeout value, so that after a PTF test is generated, a user can replace this number in one place in the PTF source file, rather than many places.

Similarly if there are other timeouts to wait for a different reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: 0.1 sec might work right now for BMv2, but I've been testing with other environments where it is definitely not long enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the environments where issues are coming up?

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fruffy fruffy merged commit 7261906 into main Jun 27, 2023
@fruffy fruffy deleted the fruffy/benchmarking_fixes branch June 27, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tools Topics related to the P4Tools back end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants